Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl: first pass limit when allocating buffer for certificates #7131

Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5188

Describe changes:

  • Adds a first time check before allocating much memory to store a certificate

Feedback welcome here

With this check, on the first packet of a certificate presenting
a length of 16Mbytes, we only allocate up to 65Kb

When we get to the point where need more than 65Kb, we realloc
to the true size.

With this check, it makes it more expensive for an attacket to use
this allocation as a way to trigger ressource exhaustion...
@catenacyber catenacyber requested a review from a team as a code owner March 10, 2022 14:14
@catenacyber catenacyber mentioned this pull request Mar 10, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #7131 (00dcf6d) into master (3a490fb) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7131      +/-   ##
==========================================
+ Coverage   78.06%   78.09%   +0.02%     
==========================================
  Files         628      628              
  Lines      185266   185271       +5     
==========================================
+ Hits       144635   144684      +49     
+ Misses      40631    40587      -44     
Flag Coverage Δ
fuzzcorpus 60.08% <100.00%> (+0.09%) ⬆️
suricata-verify 54.59% <0.00%> (+<0.01%) ⬆️
unittests 63.12% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -1431,6 +1431,10 @@ static int EnsureRecordSpace(SSLStateConnp *curr_connp, const uint8_t * const in
SCLogDebug("cert_len unknown still, create small buffer to start");
certs_len = 256;
}
// Limit in a first time allocation for very large certificates
if (certs_len > 0x10000 && certs_len > curr_connp->trec_pos + input_len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we first check if current input will set the limited value?

Also please use a macro with a clear name, a decimal value and a comment to explain the default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we first check if current input will set the limited value?

I am not sure I understand your question.
This is a first check that current input will set the limited value...

That is if not certs_len > curr_connp->trec_pos + input_len that means that the current input will cross the 65k boundary, and thus, we do not limit ourselves

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 6537

@catenacyber
Copy link
Contributor Author

Replaced by #7139

@catenacyber catenacyber mentioned this pull request Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants